Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module builder #48

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add module builder #48

wants to merge 7 commits into from

Conversation

pedorich-n
Copy link
Contributor

This PR adds a module interface to the builder derivation. This brings benefits such as strict options definition, better type-checks, and flexibility for future improvements or modifications.
I left the old builder in place for backward compatibility. In the future, it could be removed, and an "adapter" could be installed to convert old inputs into the new module system and maintain backward compatibility.

The module is inspired by @tomeon's nix-openwrt-imagebuilder-module project, but it's structured differently and the module itself is simpler. Please let me know if you feel it isn't fair to your work.

Opening this as a draft to gather opinions.
If everything is fine I will add module documentation and update README to tell users to use build-module in a follow-up PR to not pollute this one.

I also moved examples into a dedicated folder, since we have 4 of them now.

Closes #22

Copy link
Contributor Author

@pedorich-n pedorich-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would love for people to test this, maybe I missed some corner cases.
In my tests, I tried building with packages, folders, and individual files. I haven't tried disabling services, removing packages or anything else.

'';
};

subtarget = lib.mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called it subtarget instead of variant, because it seems like OpenWRT uses this name.

builder/module-options.nix Outdated Show resolved Hide resolved
Comment on lines +15 to +27
# TODO: move arch to profiles
detectedArch = hashedTarget.packagesArch;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arch is already a property in profiles.json. I feel like it's more fitting to get this info from there, than from the feeds, since it's a property of a specific board/target

@astro
Copy link
Owner

astro commented Dec 1, 2024

Cc: @ehmry @adamcstephens @MakiseKurisu

@ehmry
Copy link
Contributor

ehmry commented Dec 2, 2024

I'd like to review but I haven't been using the imagebuilder (or openwrt).

@astro
Copy link
Owner

astro commented Dec 27, 2024

I like this. Please feel encouraged to follow through.

@pedorich-n
Copy link
Contributor Author

Thanks for your encouragement, @astro !

I've updated the docs module, exposing both markdown and json. At a certain iteration, I realized it looked almost like https://github.com/astro/microvm.nix/blob/2ae08de8e8068b00193b9cfbc0acc9dfdda03181/pkgs/doc.nix, so I just copied most of the stuff from there 😅
@SuperSandro2000 please have a look.

I also added support for "separate Kmods" and tested the build with both "old" (23.05.5) and "new" (24.10.0-rc4) versions. Builds fine.

@pedorich-n pedorich-n marked this pull request as ready for review January 5, 2025 07:24
@pedorich-n
Copy link
Contributor Author

Happy New Year everybody!

I've added a new package to generate/update module docs and a GitHub flow to do it from GHA.
I've also updated the documentation with a minimal example and a recommendation to use the builder module over the builder function.

I think this PR is ready for a proper review now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR to support modular interface?
4 participants